feat(core): pass client logger to request handlers#2170
Conversation
TypeScript strict mode cannot narrow `value` from the full `NodeHttpHandlerOptions[typeof key]` union when branching on `key === "logger"`. Add an explicit cast to fix the build:types target in CI.
aBurmeseDev
changed the title
| export const getHttpHandlerExtensionConfiguration = <HandlerConfig extends object = {}>( | ||
| runtimeConfig: HttpHandlerExtensionConfigType<HandlerConfig> | ||
| ) => { | ||
| runtimeConfig.httpHandler?.updateHttpClientConfig("logger" as keyof HandlerConfig, (runtimeConfig as any).logger); |
There was a problem hiding this comment.
Add a truthy check?
| runtimeConfig.httpHandler?.updateHttpClientConfig("logger" as keyof HandlerConfig, (runtimeConfig as any).logger); | |
| if ((runtimeConfig as any).logger) { | |
| runtimeConfig.httpHandler?.updateHttpClientConfig("logger" as keyof HandlerConfig, (runtimeConfig as any).logger); | |
| } |
There was a problem hiding this comment.
added the truthy check so we don't call updateHttpClientConfig with undefined/null
| }); | ||
|
|
||
| it("updates config", async () => { | ||
| it("does not overwrite an existing logger via updateHttpClientConfig", async () => { |
There was a problem hiding this comment.
Why should updateHttpClientConfig skip overriding existing logger?
There was a problem hiding this comment.
updateHttpClientConfig should always overwrites, removed the nullish assignment.
| export const getHttpHandlerExtensionConfiguration = <HandlerConfig extends object = {}>( | ||
| runtimeConfig: HttpHandlerExtensionConfigType<HandlerConfig> | ||
| ) => { | ||
| runtimeConfig.httpHandler?.updateHttpClientConfig("logger" as keyof HandlerConfig, (runtimeConfig as any).logger); |
There was a problem hiding this comment.
the method call must be checked too. ?.
try requiring HandlerConfig extends { logger?: Logger } instead of casting a literal to something it isn't.
This should be backwards compatible.
interface Logger {
log(...args: unknown[]): void;
}
interface RequestHandler<C> {
update(key: keyof C, val: unknown): void;
}
function callUpdate<C extends ({ logger?: Logger })>(handler: RequestHandler<C>) {
handler.update("logger", null);
}
const handler: RequestHandler<{}> = null as any;
callUpdate(handler);There was a problem hiding this comment.
added ?. on the method and Logger import with the constraint (HandlerConfig extends { logger?: Logger }).
Also wrapped in a truthy check, so we skip the call entirely when there's no logger.
| this.configProvider = this.configProvider.then((config) => { | ||
| if (key === "logger") { | ||
| return { ...config, logger: config.logger ?? (value as NodeHttpHandlerOptions["logger"]) }; | ||
| } |
There was a problem hiding this comment.
this makes it impossible to update the logger once it's set
There was a problem hiding this comment.
removed the nullish assignment. updateHttpClientConfig now does a plain overwrite for all keys including logger.
kuhe
left a comment
kuhe
left a comment
There was a problem hiding this comment.
look at additional requestHandler implementations in AWS SDK JS.
at this time, handlers which resolve config asynchronously don't offer a way to query whether there is a higher priority logger set on the requestHandler at requestHandler initialization.
We need a plan to account for this problem.
3 participants
Issue #, if available:
aws/aws-sdk-js-v3#6130
JS-7027
Description of changes:
Pass the client's logger to the HTTP request handler via
updateHttpClientConfigso it can log connection-level diagnostics (warnings, debug info, etc).Testing
getHttpHandlerExtensionConfigNodeHttpHandler.updateHttpClientConfignullishUndiciHttpHandler.updateHttpClientConfigBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.